Addresses comments in runtime feature discovery API #13964

larroy · 2019-01-23T01:50:58Z

Description

Returns a struct of LibFeature and reduces to a single API call querying of compile-time features.

Followup on API comments on #13549

@szha

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

…n API

szha · 2019-01-23T02:03:32Z

@larroy thanks! will take a look shortly.

include/mxnet/mxruntime.h

tests/cpp/misc/runtime_test.cc

tests/python/unittest/test_runtime.py

include/mxnet/mxruntime.h

include/mxnet/c_api.h

python/mxnet/runtime.py

szha

To me, it seems that there are new interfaces that can be combined together that can still achieve the same goal easily. Further minimizing those would be desirable.

larroy · 2019-01-23T10:59:52Z

Seems CI status is fully missing.

szha · 2019-01-25T18:06:43Z

What installation dependencies are you referring to? Libinfo is responsible for the information on libmxnet hence the suggestion.

larroy · 2019-01-25T20:58:53Z

@szha I did the changes that you requested, but doesn't work due to libinfo.py being used before loading the library, is a rabbit hole, see this branch: https://github.com/larroy/mxnet/tree/feature_discovery_libinfo

I discarded that idea. Can we provide a path to completion?

szha · 2019-01-25T21:42:34Z

@larroy I will need more information to offer advice on how to make it work. Alternatively, I can fork the branch and try to make it work. Let me know what you prefer.

Update:
I took a look at the branch and saw the circular dependency issue. While there are workarounds, they either require atypical import or cross module registration, making it harder to maintain. For simplicity, let's keep the frontend module in mx.runtime and the feature enum implementation in libinfo.h.

larroy · 2019-01-30T08:27:06Z

I have been thinking more about this and I would have the following proposal to address your concerns:

1- Change the name of the implementation files and python module to runtimeinfo.h and mxnet.runtimeinfo in python. I would like to have the same name in the C api and bindings for consistency. And since we can't use libinfo we need to use something else.
2- Leave the API calls as they are because the changes we discussed increase complexity either requiring memory allocation or getting the number of elements, etc. It's super simple to get the list of features and query a flag, these are two API calls. We can do with a single one but it requires what in my opinion is overengineering by returning an array of structs, each struct with a string. I think is better practice to have more simple API calls that do one thing than less calls that do many things. (A bad example of this is the posix file API for example, few calls, many ways to use it, complex). Interface segregation is usually a good practice and to me checking a simple boolean flag with no allocations is beautiful. Let me know if you feel strongly about this, or if this proposal would work for you to accept the PR.

Pedro.

szha · 2019-01-30T21:15:59Z

since the information is fixed after compilation, libinfo sounds more accurate. there's no need to maintain the consistency in naming between module name of python frontend and the file name in the backend.

On the API design, yes, I feel strongly about the change, hence the change request. The reason is that the library information doesn't change after it's compiled, and since it's loaded only once, there's no futher need to query the backend, especially given that ctypes isn't exactly fast. Thus, having an API that queries all feature status at the beginning, and make that information available should be the goal. To achieve that goal, a single method with complete information should be the way to go.

…_libinfo

larroy · 2019-02-12T10:05:46Z

Thanks for the merge @szha

* Prototype for runtime feature detection * Includes from diamond to quotes * Add CPU feature and BLAS flavour flags * Add BLAS flavour and CPU SSE and AVX flags * MXNET_USE_LAPACK * Fix C++ linting errors * Expose runtime feature detection in the public C API and in the Python API * Refactor Storage -> FeatureSet * Refine documentation * Add failure case * Fix pylint * Address CR comments * Address CR comments * Address CR * Address CR * Address CR * Address CR * remove old files * Fix unit test * Port CMake blas change from apache#13957 * Fix lint * mxruntime -> libinfo * Fix comments * restore libinfo.py * Rework API for feature detection / libinfo * Refine documentation * Fix lint * Fix lint * Define make_unique only for C++ std < 14 * Add memory include * remove old tests * make_unique fiasco * Fix lint

larroy and others added 13 commits January 16, 2019 14:43

Prototype for runtime feature detection

ff109f5

Includes from diamond to quotes

5ebc7a2

Add CPU feature and BLAS flavour flags

30f057f

Add BLAS flavour and CPU SSE and AVX flags

e9eff40

MXNET_USE_LAPACK

2e60d23

Fix C++ linting errors

d913033

Expose runtime feature detection in the public C API and in the Pytho…

939d76c

…n API

Refactor Storage -> FeatureSet

80ae05e

Refine documentation

c6bdb54

Add failure case

a27af10

Fix pylint

799a567

Address CR comments

ba356cf

Address CR comments

09e68a6

larroy requested review from anirudh2290 and szha as code owners January 23, 2019 01:50